sec(auth): stop leaking MFA enrollment via login error responses (closes #388)#886
sec(auth): stop leaking MFA enrollment via login error responses (closes #388)#886cristim wants to merge 1 commit into
Conversation
cristim
added
triaged
|
@coderabbitai review |
📝 WalkthroughWalkthroughThe PR adds a machine-detectable ChangesMFA not-configured error handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
✅ Actions performedReview triggered.
|
|
Rebased Conflict resolution: only Prod-side files ( Verification on the rebased tree: gofmt clean, No prior CodeRabbit findings to address: the only earlier CR activity on this PR was a per-PR review-rate-limit notice (2026-05-30), so this is the first substantive review request. @coderabbitai review |
|
🧠 Learnings used✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
#388) The login handler previously forwarded err.Error() verbatim for any auth failure not matching ErrMFARequired or ErrInvalidMFACode. This included "MFA is enabled but not configured", which revealed to an attacker that credentials were correct and MFA was enrolled (but broken). Two attack vectors were closed: 1. Add ErrMFANotConfigured sentinel to internal/auth/errors.go and return it (wrapped via %w) from verifyPasswordAndMFA. Map it to "mfa_required" in the login handler -- identical to ErrMFARequired -- so "MFA enrolled + working" is indistinguishable from "MFA enrolled + broken secret" in the HTTP response. 2. Replace the bare err.Error() fallthrough in the login handler with a fixed "invalid credentials" string so no internal error message ever reaches the client. Tests positively assert response equivalence: ErrMFARequired and ErrMFANotConfigured produce the same 401 + "mfa_required" body; all wrong-password paths produce the same 401 + "invalid credentials" body.
|
Rebased on Single content conflict, resolved as an additive merge in Post-rebase: gofmt clean, go vet clean, build clean, @coderabbitai review |
|
🧠 Learnings used✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/api/handler_auth.go`:
- Around line 60-63: The current handler_auth.go maps every error returned from
auth.Login to NewClientError(401, "invalid credentials"), hiding non-auth
issues; change the logic in the code that calls auth.Login to only return the
opaque 401 for the explicit normalized credential-failure sentinel (e.g.,
auth.ErrInvalidCredentials or whatever specific value/type the auth package
uses), and for any other error returned by auth.Login let it propagate or map to
an appropriate code (e.g., return the original error, convert validation errors
to 400, and server-side errors to 500) instead of collapsing them into 401;
update the conditional that currently does return nil, NewClientError(401,
"invalid credentials") to check the concrete error identity/type first and
handle other errors separately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 512222fa-9809-4e32-afdb-628721e40d02
📒 Files selected for processing (5)
internal/api/handler_auth.gointernal/api/handler_auth_test.gointernal/auth/errors.gointernal/auth/service.gointernal/auth/service_test.go
| // All other auth failures (wrong password, account not found, | ||
| // locked, etc.) collapse to a single opaque 401. Never forward | ||
| // err.Error() verbatim — it may reveal internal account state. | ||
| return nil, NewClientError(401, "invalid credentials") |
There was a problem hiding this comment.
Don't collapse unexpected login failures into 401s.
auth.Login can still return non-auth errors like invalid email format and failed to create session: ... from internal/auth/service.go. Mapping all of those to 401 invalid credentials will hide real outages and misclassify bad requests as auth failures. Keep the opaque 401 only for the normalized credential-failure path, and let everything else bubble or map separately.
Suggested fix
- // All other auth failures (wrong password, account not found,
- // locked, etc.) collapse to a single opaque 401. Never forward
- // err.Error() verbatim — it may reveal internal account state.
- return nil, NewClientError(401, "invalid credentials")
+ // Only the normalized credential-failure path should collapse to
+ // the opaque 401. Let validation/infrastructure errors preserve
+ // their semantics upstream.
+ switch err.Error() {
+ case "invalid email or password":
+ return nil, NewClientError(401, "invalid credentials")
+ case "invalid email format":
+ return nil, NewClientError(400, "invalid email format")
+ default:
+ return nil, err
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // All other auth failures (wrong password, account not found, | |
| // locked, etc.) collapse to a single opaque 401. Never forward | |
| // err.Error() verbatim — it may reveal internal account state. | |
| return nil, NewClientError(401, "invalid credentials") | |
| // Only the normalized credential-failure path should collapse to | |
| // the opaque 401. Let validation/infrastructure errors preserve | |
| // their semantics upstream. | |
| switch err.Error() { | |
| case "invalid email or password": | |
| return nil, NewClientError(401, "invalid credentials") | |
| case "invalid email format": | |
| return nil, NewClientError(400, "invalid email format") | |
| default: | |
| return nil, err | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/api/handler_auth.go` around lines 60 - 63, The current
handler_auth.go maps every error returned from auth.Login to NewClientError(401,
"invalid credentials"), hiding non-auth issues; change the logic in the code
that calls auth.Login to only return the opaque 401 for the explicit normalized
credential-failure sentinel (e.g., auth.ErrInvalidCredentials or whatever
specific value/type the auth package uses), and for any other error returned by
auth.Login let it propagate or map to an appropriate code (e.g., return the
original error, convert validation errors to 400, and server-side errors to 500)
instead of collapsing them into 401; update the conditional that currently does
return nil, NewClientError(401, "invalid credentials") to check the concrete
error identity/type first and handle other errors separately.
|
pr-iterate auto-rebase stopped: genuine semantic conflict (not the expected additive test merge). The latest merge wave landed #830 (
Both close a real leak but pick opposite indistinguishability targets, and each ships a test asserting its own outcome (base: This needs a human security decision on which leak model wins before the rebase can proceed: Rebase was aborted; branch is unchanged at 804ff30. No force-push, no CR re-ping. Re-run pr-iterate after the direction is chosen. |
|
Closing as superseded. The MFA-enrollment leak this targets (#388) is already fixed by the merged PR #830 (closes #391), which collapses the missing-MFA-secret branch into the generic wrong-credentials response - a strictly stronger anti-enumeration posture than this PR's typed mfa_required response (it hides even that MFA is enrolled for the broken-secret case). The service-layer ErrMFANotConfigured -> mfa_required approach here directly conflicts with #830's generic collapse and cannot coexist with it. The one genuinely-unique improvement in this PR - replacing the handler_auth.go login catch-all 'return NewClientError(401, err.Error())' with an opaque 'invalid credentials' so raw service errors are never forwarded - is being carried forward as a small standalone hygiene follow-up issue. Tracking there. |
…ror() (closes #937) (#938) The login() catch-all error path was forwarding raw service errors verbatim via NewClientError(401, err.Error()), potentially exposing internal account state (e.g. lock status, user IDs, timestamps) to unauthenticated callers. Replace it with the opaque hardcoded string "invalid credentials" so all non-sentinel auth failures (wrong password, account not found, locked, etc.) collapse to the same response, regardless of what the service returns. The typed-sentinel arms above (ErrMFARequired -> "mfa_required", ErrInvalidMFACode -> "invalid_mfa_code") are unchanged. Adds TestHandler_login_OpaqueError_HidesInternalMessage as a regression test that pins the service returning a distinctive internal message and asserts the 401 body is "invalid credentials" with no internal detail leaking through. Salvaged from closed #886.
1 participant
Summary
ErrMFANotConfiguredsentinel tointernal/auth/errors.goand wrap it inverifyPasswordAndMFA. Map it to"mfa_required"in the login handler (same asErrMFARequired) so an attacker cannot distinguish "MFA enrolled and working" from "MFA enrolled but secret missing" via the 401 response.err.Error()fallthrough in the login handler with a fixed"invalid credentials"string so no internal error message ever reaches the client.ErrMFANotConfiguredmessage string is identical to the pre-sentinel literal, somapMFAServiceError's substring fallback still fires correctly until refactor(auth/mfa): use sentinel errors in mapMFAServiceError #883 lands.Test plan
TestLogin_FailedAuth_ResponseEquivalencepositively asserts that wrong-password for a no-MFA user and wrong-password for an MFA-enrolled user both return HTTP 401 +"invalid credentials"-- identical bodies.TestLogin_MFANotConfigured_ReturnsMFARequiredassertsErrMFANotConfiguredproduces 401 +"mfa_required", not the internal error string.TestLogin_MFARequired_And_NotConfigured_ProduceSameResponsecross-checksErrMFARequiredandErrMFANotConfiguredproduce the exact same status code and response body.TestLogin_WithMFA_NoSecret(auth package) updated to useerrors.Is(err, ErrMFANotConfigured)-- a stronger assertion thanContains.go test ./internal/auth/... ./internal/api/...-- 1857 tests pass.Summary by CodeRabbit
Release Notes